ci: check the public type surface with api-extractor - #777
Conversation
4.2.4 shipped a breaking type change as a patch. ObservableStatus<T> went from a flat interface to a discriminated union, so data became T | undefined and the documented destructure-and-use pattern stopped compiling for strict-TS consumers. Nothing in the release flow compared the published type surface, so it went out unnoticed. api-extractor generates etc/reactfire.api.md, a report of every public export. CI regenerates it and fails when it no longer matches, so a change to the type surface shows up as a reviewable diff in the pull request instead of being discovered by a consumer after release. The report is generated by npm run api:report and never hand-edited. Nothing here is maintained by hand. Verified against a faithful reproduction of 4.2.4, patched so only consumers break rather than internals: plain tsc still passes, and api:check exits 1 with a one-line diff, data: T becoming T | undefined. Two notes for review: api-extractor bundles TypeScript 5.9.3 and this repo is on 6.0.3, so it prints a version warning on every run. The emitted .d.ts contain no TypeScript 6 only syntax, and the report reproduces the emitted signatures exactly, so the older engine has nothing to misparse. Worth rechecking if the public surface ever adopts newer syntax. The job builds with vite, not tsc. tsconfig.json sets noEmit, so tsc is a type check only; @rollup/plugin-typescript inside the vite build is what emits the declarations api-extractor reads. Closes FirebaseExtended#749
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
armando-navarro
left a comment
armando-navarro
left a comment
There was a problem hiding this comment.
Thanks Tyler. This is a clean implementation of the #749 net, and the api-extractor choice over a published-.d.ts differ is well argued (no network, no dependency on what latest points at). I am approving. One thing to reconcile before merge:
What I verified
I confirmed the gate is load-bearing rather than decorative, which was my main question:
- On the branch,
npm ci+npx vite build+npm run api:checkpasses (exit 0), and the report matches the emitted declarations. - I reintroduced the exact 4.2.4 regression in
src/useObservable.ts(data: Ttodata: T | undefined), rebuilt, andapi:checkexited 1. Restoring it went back to green. So a real breaking type change fails CI, not just a hand-edit of the report. Worth noting the failure prints as "completed with warnings", but the process exit code is 1, so the step does fail. - The generated
etc/reactfire.api.mdfaithfully reproduces the emitted surface (includingObservableStatus<T>withdata: T), thepackage-lock.jsonadditions are all api-extractor's own dependency subtree with no other version changes, and there are nosrc/docschanges, so the PR does not itself move the public API. - The
publishjob correctly gains a hard dependency oncheck-api, and the base is right:verify-package/verify-loadsonly exist on #766's branch, so this does need to land after #766.
I also checked the TypeScript-skew note (bundled 5.9.3 vs 6.0.3) and agree it is cosmetic today, the report reproduces the 6.0.3 signatures exactly.
One question on the #749 reference
The commit message body ends with Closes #749, while the PR description says Refs #749. Those two disagree, and since this is a single commit, the merge puts that commit onto main carrying Closes #749, so the commit text is what lands, not the description's Refs.
I do not have a strong view on which way it should go. Reading #749, it asks for this kind of type-surface safety net (it sketches a published-version npm pack diff and this PR uses a committed api-extractor report instead, but the intent is the same), and it puts the live 4.2.4 remediation under "tracked separately", so I can see the case for closing it here. That call is yours. I mainly want to flag that the commit and the description point in opposite directions, so whichever you intend, it would be good to make them match rather than leave the outcome to whichever text GitHub acts on.
2 participants
Adds an api-extractor report so a change to the public type surface shows up as a reviewable diff instead of being discovered after publish.
Refs #749. Deliberately not
Closes: #749 also covers the already-shipped 4.2.4 and 4.2.5, which neednpm deprecateand are not addressed here. #749 should be closed by hand once that half is done.Why this rather than a published-.d.ts differ
The original plan was to diff the emitted
.d.tsagainst the last published tarball. api-extractor closes the same gap without the network:data: T;todata: T | undefined;). The fix: update useFirestoreDocData return type to include undefined #733 shape would have surfaced too.latesthappens to point at. That matters here, because comparing a candidate againstlatestbreaks the moment a lower major ships: 5.0.0 takinglatestwould have failed the whole v4 line.What it does not do
api-extractor does not classify additive vs breaking. It shows the diff; a human decides which it is. That is the honest limit of this check.
expectTypeOftype tests were considered and dropped rather than forgotten: hand-maintained, asymmetric (assertions on one interface, nothing on the other ~70 exports), and V5 will churn these exact types during the freeze.Verification
api:checkpasses on this base. Worth stating because the base moved: fix(types): use explicit .js extensions in relative imports #770 put explicit.jsextensions in the emitted.d.ts, and that turned out not to perturb the report.etc/reactfire.api.mdmakesapi:checkexit 1 withYou have changed the API signature for this project. Restoring it returns to exit 0.temp/(api-extractor scratch output) is gitignored.Note on the TypeScript skew
api-extractor bundles TS 5.9.3; the repo is on 6.0.3, and it prints a warning saying so. Settled 07-30: the report reproduces TS 6.0.3's emitted signatures character for character, and the emitted
.d.tscontain no TS 6 only syntax. Regenerating and diffing against itself proves nothing (same engine, same source: determinism, not correctness). Recheck if the public surface adopts newer syntax.Base
Targets
chore/built-artifact-gate(#766) so this shows only its own changes. It will auto-retarget tomainwhen #766 merges.